Hmm, I think this is problematic. bt_link_mask is initialized with "check_nhlt_ssp_mask(sdev, NHLT_DEVICE_BT)" and as BT link is enabled on various Windows devices where we do not have support in Linux (and we have already shipped tplgs+kernel for these devices and the devices themselves are shipping). So adding "-bt" based on this can break a lot of existing devices.

Although, I must say "can break" as I note commit b28b23d introduces this for for HDA devices. And indeed, when I test on a HDA machine, an extra DAI link is created:

[12443.363488] snd_soc_intel_sof_board_helpers:set_bt_offload_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 8: bt offload, ssp 1

... this seems to still work as topology does not use it, and this is the last link, so extra "be_id" doesn't create harm.
Have you @bardliao @ujfalusi noted this before?

Given only Chromebook topologies have had the PCM definition for BT offload, I think we need an opt-in mechanism:
a) if a legacy board case with in all topologies, no need to add "-bt" (the legacy option, used primarily for Chromebooks)
b) if new platform, board has BT link set in NHLT, add "-bt"
c) if new platform, board has DMI quirk to enable, add "-bt" (used for new Chromebooks)

We can continue to use (a) for I2S platforms, but for SDW topologies, we'd need to transition to b+c. Not sure what "new platform" should be. Perhaps a platform not yet shipping (NVL-S perhaps) so we have time to add the necessary "-bt" variants, and ship them. At least then we are not breaking anyone's existing system when the kernel starts looking for "-bt.tplg" variants.

Here's how it looks on a generic HDA machine where BIOS NHLT has BT links defined:

[13927.913567] snd_soc_skl_hda_dsp:skl_hda_audio_probe: skl_hda_dsp_generic skl_hda_dsp_generic: board_quirk = 4008000
[13927.913573] snd_soc_intel_sof_board_helpers:sof_intel_board_get_ctx: skl_hda_dsp_generic skl_hda_dsp_generic: create ctx, board_quirk 0x4008000
[13927.914974] snd_soc_intel_sof_board_helpers:sof_intel_board_set_dai_link: skl_hda_dsp_generic skl_hda_dsp_generic: create dai links, link_order 0x63284, id_overwrite 0x87641
[13927.914977] snd_soc_intel_sof_board_helpers:set_idisp_hdmi_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 1: idisp hdmi 1, idisp codec 1
[13927.914979] snd_soc_intel_sof_board_helpers:set_idisp_hdmi_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 2: idisp hdmi 2, idisp codec 1
[13927.914982] snd_soc_intel_sof_board_helpers:set_idisp_hdmi_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 3: idisp hdmi 3, idisp codec 1
[13927.914984] snd_soc_intel_sof_board_helpers:set_hda_codec_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 4: hda analog
[13927.914985] snd_soc_intel_sof_board_helpers:set_hda_codec_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 5: hda digital
[13927.914986] snd_soc_intel_sof_board_helpers:set_dmic_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 6: dmic01
[13927.914987] snd_soc_intel_sof_board_helpers:set_dmic_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 7: dmic16k
[13927.914988] snd_soc_intel_sof_board_helpers:set_bt_offload_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 8: bt offload, ssp 2

Hmm, I think this is problematic. bt_link_mask is initialized with "check_nhlt_ssp_mask(sdev, NHLT_DEVICE_BT)" and as BT link is enabled on various Windows devices where we do not have support in Linux (and we have already shipped tplgs+kernel for these devices and the devices themselves are shipping). So adding "-bt" based on this can break a lot of existing devices.

Yes, that is my main concern.

Although, I must say "can break" as I note commit b28b23d introduces this for for HDA devices. And indeed, when I test on a HDA machine, an extra DAI link is created:

[12443.363488] snd_soc_intel_sof_board_helpers:set_bt_offload_link: skl_hda_dsp_generic skl_hda_dsp_generic: link 8: bt offload, ssp 1

... this seems to still work as topology does not use it, and this is the last link, so extra "be_id" doesn't create harm. Have you @bardliao @ujfalusi noted this before?

Yes, I reviewed the PR. It basically creates the dai links based on the board_quirk.

Given only Chromebook topologies have had the PCM definition for BT offload, I think we need an opt-in mechanism: a) if a legacy board case with in all topologies, no need to add "-bt" (the legacy option, used primarily for Chromebooks) b) if new platform, board has BT link set in NHLT, add "-bt" c) if new platform, board has DMI quirk to enable, add "-bt" (used for new Chromebooks)

We can continue to use (a) for I2S platforms, but for SDW topologies, we'd need to transition to b+c. Not sure what "new platform" should be. Perhaps a platform not yet shipping (NVL-S perhaps) so we have time to add the necessary "-bt" variants, and ship them. At least then we are not breaking anyone's existing system when the kernel starts looking for "-bt.tplg" variants.

So the -bt suffix only apply if .hw_ip_version >= SOF_INTEL_ACE_4_0, . And we have the tplg_filename module parameter which can be used when a legacy board has with BT and without BT variants. Does it sound good?

it does not matter what is used for the BT link? Which SSP for example.
In a generic machine you will need special user space to be able to use the BT PCM, right? What happens if PW opens the BT link when probing the card? Will that cause it to fail and the card being rejected?

Good point @ujfalusi , it's going to be SSP2 in majority of cases, but we if we start adding the suffix, let's add the ssp id as well, So "-ssp%u-bt" matching the convention we use in topology (bt-defaults.conf -> "BT_NAME").
UPDATE: to match with exisiting topologies, should be "-bt-ssp%u" (like sof-ptl-es8336-ssp1.tplg, sof-ptl-es83x6-ssp1-hdmi-ssp02 and so forth).

Otherwise, if the link is enabled in BIOS, then it's ok, the PCM can be opened. Streaming will work with some parameters (A2DP 48kHz), but for others not (8/16 SCO mode) as there will be no clock provided by the BT chip. But this should be enough for PW, it will just open the PCMs and query capabilities.

This looks good to me @bardliao @ujfalusi . We just then need to remember this when doing the topologies for ace4 platforms.

This should be "%s-ssp%d-bt".

This should be "%s-ssp%d-bt".

Good catch. Thanks @macchian

LGTM. Verified sof_tplg_name-ssp%d-bt.tplg can be loaded.